Skip to content

Remove a false performance claim in addRowMultiple, and the dead macros behind it - #77

Merged
d-torrance merged 3 commits into
Macaulay2:masterfrom
d-torrance:f4-inner-loop-comments
Sep 1, 2026
Merged

d-torrance merged 3 commits into
Macaulay2:masterfrom
d-torrance:f4-inner-loop-comments

Conversation

@d-torrance

Copy link
Copy Markdown
Member

The comment on the entries local in DenseRow::addRowMultiple claimed that
indexing mEntries directly instead would cost 14% of the whole matrix
reduction. That was measured on MSVC 2012 in 2013 and never since; its own
author wrote "That does not make sense to me, but it is a fact none-the-less."

It is not a fact on any compiler in use. Measured with the loop built both ways
and timed in interleaved runs against hyclic8-101-trimmed, yang1 and
hilbertkunz1:

  • GCC 11.4 -O2, x86-64 emits different code with and without the local,
    and times the two within −0.9% to +1.4% — inside the run-to-run noise on all
    three inputs.
  • clang 21, arm64 emits byte-identical object files either way, so there is
    nothing there to time at all.

MSVC cannot build this code regardless: stdinc.h has required C++17 since
PR #60, and MSVC 2012 does not do complete C++11. There is no compiler left for
which the claim could hold.

Removing the local leaves MATHICGB_RESTRICT with no users, and auditing its
neighbours in the same three compiler blocks found five more unused macros —
four of which have never been syntactically valid on GCC or clang
(__attribute__(x) where the syntax needs __attribute__((x))), plus one with
its while(0) inside the do block instead of closing it. The first use of any
of them would be a build failure. They have been that way since 2013 and nothing
noticed, because nothing uses them.

Commits

Each builds standalone and passes the full suite, so the series is bisectable.

  1. 0190b09 — drop the restrict local and the assert that only checked
    vector::data() points where operator[] does
  2. 6084a28 — remove MATHICGB_RESTRICT, now unused
  3. 555dec3 — remove the five dead macros

On the API surface

stdinc.h is installed, so removing public macros is nominally an API removal,
with no ABI effect. Macaulay2 is the only known consumer and uses none of them:
grepping its tree for all six names returns nothing outside the mathicgb
submodule, and the only MATHICGB_* identifiers it references anywhere are
MATHICGB_LIBRARIES, MATHICGB_INCLUDE_DIR, MATHICGB_FOUND,
MATHICGB_VERSION_STRING, MATHICGB_DEBUG and MATHICGB_NO_TBB — things it
sets or probes for, not attributes it consumes. It also cannot reach these
definitions: it includes only mathicgb.h and mathicgb/mtbb.hpp, neither of
which includes stdinc.h.

Deliberately left alone

MATHICGB_ASSUME and MATHICGB_CONCATENATE also have no uses outside
stdinc.h, and both are load-bearing. MATHICGB_ASSUME is the non-debug
definition of MATHICGB_ASSERT, so removing it would break all 898 assertions
in release builds. MATHICGB_CONCATENATE backs
MATHICGB_CONCATENATE_AFTER_EXPANSION, which MATHICGB_UNIQUE needs. The
asymmetry in commit 3 is intentional: the macro whose comment begins "As
MATHICGB_ASSUME, but…"
goes while MATHICGB_ASSUME itself stays.

The manual unrolling below the removed comment is a separate question and is
untouched. It was measured in the same pass and does earn its keep — replacing
it with a plain loop costs 8–12% on hyclic8-101-trimmed, though nothing on
yang1.

Verification

246/246 tests pass at each of the three commits. hyclic8-101-trimmed.gb is
byte-identical to before. The build with the local dropped times at −0.1%
against the build with it, sd 0.6–1.0% over 15 interleaved reps.

🤖 Generated with Claude Code

d-torrance and others added 3 commits August 31, 2026 14:07
The comment on it claimed that indexing mEntries directly instead would cost
14% of the whole matrix reduction, measured on MSVC 2012 in 2013 and never
since.  Its own author wrote "That does not make sense to me, but it is a fact
none-the-less."

It is not a fact on any compiler in use.  Measured in 2026 with the loop built
both ways and timed in interleaved runs against hyclic8-101-trimmed, yang1 and
hilbertkunz1: GCC 11.4 -O2 on x86-64 emits different code with and without the
local but times the two within -0.9% to +1.4%, inside the run-to-run noise on
all three inputs.  clang 21 on arm64 emits byte-identical object files either
way, so there is nothing there to time at all.

MSVC cannot build this code regardless -- stdinc.h has required C++17 since
PR #60, and MSVC 2012 does not do complete C++11 -- so there is no compiler
left for which the claim could still hold.

Dropping it takes the assert with it.  MATHICGB_ASSERT(entries + it.index() ==
&mEntries[it.index()]) only checked that vector::data() points where
operator[] does, which is a statement about std::vector rather than about this
code.

Verified: 246/246 tests pass, hyclic8-101-trimmed.gb is unchanged, and the
build with the local dropped times at -0.1% against the build with it, sd
0.6-1.0% over 15 interleaved reps.

The manual unrolling below is a separate question and is left alone.  It was
measured in the same pass and does earn its keep: replacing it with a plain
loop costs 8-12% on hyclic8-101-trimmed, though nothing on yang1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The restrict local in DenseRow::addRowMultiple, removed in the previous
commit, was the only place it was ever used.

All three definitions go, not just the one this build takes.  The macro is
defined once per compiler branch -- __restrict for MSVC, __restrict for
GCC/clang, and empty for the fallback -- and removing only one branch would
leave a macro that exists on some compilers and not others, which is worse
than either keeping or removing it.

stdinc.h is installed by both build systems, so this is nominally an API
removal, as PR #60's FlattenNamespace change was.  There is no ABI effect: a
macro emits no symbol.  Nothing outside the project has reason to use a
MATHICGB_-prefixed compiler-portability macro, and anything that did would
already have had to cope with it expanding to nothing on unrecognized
compilers.

Verified: 246/246 tests pass.

Several neighbouring macros in the same blocks look equally unused, and some
are visibly broken -- __attribute__(pure) is missing its inner parentheses and
would not compile if anything expanded it.  Auditing those is a separate pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MATHICGB_RESTRICT, removed in the previous commit, turned out not to be alone.
Auditing its neighbours in the same three compiler blocks found five more with
no uses anywhere in the project:

  MATHICGB_ASSUME_AND_MAY_EVALUATE
  MATHICGB_MUST_CHECK_RETURN_VALUE
  MATHICGB_NOTHROW
  MATHICGB_PURE
  MATHICGB_RETURN_NO_ALIAS

What settles it is that in the GCC/clang branch none of the five would
compile if anything did use them.  Four are written __attribute__(x) where
the attribute syntax needs __attribute__((x)):

  #define MATHICGB_RETURN_NO_ALIAS         __attribute__(malloc)
  #define MATHICGB_NOTHROW                 __attribute__(nothrow)
  #define MATHICGB_PURE                    __attribute__(pure)
  #define MATHICGB_MUST_CHECK_RETURN_VALUE __attribute__(warn_unused_result)

and the fifth has its while(0) inside the do block rather than closing it.
Checked by expanding each one in a one-line translation unit against GCC
11.4: MATHICGB_PURE gives "error: expected '(' before 'pure'", its three
siblings give the same shape, and MATHICGB_ASSUME_AND_MAY_EVALUATE gives
"error: expected primary-expression before '}' token".  So the first use of
any of them, on the compiler every current build uses, is a build failure.
They have been that way since 2013 and nothing noticed, because nothing uses
them.  A facility nobody can adopt is not worth keeping; adding one back
correctly is two lines.

The MSVC spellings look right and the fallback branch defines them empty, but
all three copies go together -- leaving a macro that exists on some compilers
and not others is worse than either keeping or removing it.  The five ///
comments in the MSVC block go too, since they document macros that no longer
exist.

stdinc.h is installed, so this is nominally an API removal, with no ABI
effect.  Macaulay2 is the only known consumer and uses none of them: grepping
its tree at 169ac29181 for all six macro names returns nothing outside our own
submodule, and the only MATHICGB_* identifiers it references anywhere are
MATHICGB_LIBRARIES, MATHICGB_INCLUDE_DIR, MATHICGB_FOUND, MATHICGB_VERSION_
STRING, MATHICGB_DEBUG and MATHICGB_NO_TBB -- things it sets or probes for,
not attributes it consumes.  It also cannot reach these definitions: it
includes only mathicgb.h and mathicgb/mtbb.hpp, neither of which includes
stdinc.h.

Two near neighbours are deliberately left alone, because the obvious grep
calls them dead and they are not.  MATHICGB_ASSUME has no uses outside
stdinc.h but is the non-debug definition of MATHICGB_ASSERT further down the
file, so removing it would break all 898 assertions in release builds -- it
survives immediately above the first hunk here, while the macro whose comment
begins "As MATHICGB_ASSUME, but..." does not, which is correct but reads
asymmetrically.  MATHICGB_CONCATENATE is used by
MATHICGB_CONCATENATE_AFTER_EXPANSION, which MATHICGB_UNIQUE needs.

Verified: 246/246 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@d-torrance
d-torrance merged commit 45872b6 into Macaulay2:master Sep 1, 2026
5 checks passed
@d-torrance
d-torrance deleted the f4-inner-loop-comments branch September 1, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant